Issue #3517035 by richardgaunt, joshua1234511, fionamorrison23: WCAG - Focus lands in wrong place upon form submission when error present #1443
Conversation
WalkthroughA Drupal accessibility enhancement adds behavior to focus the first error-summary element after page load or AJAX updates. Two JavaScript behavior files implement the focus logic; the Twig status-messages template now emits unique IDs and tabindex attributes for error messages. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Drupal
participant Behavior as ctFocusErrorSummary
participant DOM
Browser->>Drupal: Page load or AJAX complete
Drupal->>Behavior: Attach(ctFocusErrorSummary)
Note right of Behavior: wait ~30ms to allow DOM updates
Behavior->>DOM: querySelector('[id^="error-summary-"]')
alt element found
DOM-->>Behavior: element
Behavior->>DOM: setAttribute('tabindex','-1')
Behavior->>DOM: focus()
DOM-->>Browser: focus moved to error summary
else no element
DOM-->>Behavior: null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
web/themes/contrib/civictheme/assets/js/status-messages/status-messages.js (3)
6-6: Document the rationale for the 30ms delay.The 30ms timeout appears arbitrary. While a brief delay may be necessary to ensure DOM updates complete after AJAX operations, the specific value and reasoning should be documented.
Consider adding a more detailed comment:
- // Wait for DOM update after AJAX or normal load + // Wait 30ms for DOM updates to complete after AJAX or initial load. + // This ensures error summaries are fully rendered before focusing. setTimeout(function () {
10-10: Redundant tabindex assignment.Line 10 sets
tabindex="-1"on the error element, but the Twig template (line 10 of status-messages.html.twig) already includestabindex="-1"in theid_attrstring. This assignment is redundant.If the template reliably sets tabindex, this line can be removed:
if ($error.length) { - $error.attr('tabindex', '-1'); // ensure focusable (in case) $error.focus(); }However, keep it as a defensive measure if there's uncertainty about the template's attribute handling.
3-15: Consider using Drupal'sonce()API to prevent duplicate processing.The behavior will re-run on every AJAX update within the same context. While this is likely the desired behavior for form validation errors, using the
once()API would prevent the same error element from being focused multiple times if the behavior attaches repeatedly to the same DOM.Drupal.behaviors.ctFocusErrorSummary = { attach: function (context, settings) { // Wait for DOM update after AJAX or normal load setTimeout(function () { // Find any element with id starting with 'error-summary-' - var $error = $('[id^="error-summary-"]', context).first(); + var $error = $('[id^="error-summary-"]', context).once('ct-focus-error').first(); if ($error.length) { $error.attr('tabindex', '-1'); // ensure focusable (in case) $error.focus(); } }, 30); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/themes/contrib/civictheme/assets/js/status-messages/status-messages.js(1 hunks)web/themes/contrib/civictheme/civictheme_starter_kit/assets/js/status-messages/status-messages.js(1 hunks)web/themes/contrib/civictheme/templates/misc/status-messages.html.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push
- GitHub Check: build-and-push
- GitHub Check: commit
| // CivicTheme status-messages.js | ||
| (function ($, Drupal) { | ||
| Drupal.behaviors.ctFocusErrorSummary = { | ||
| attach: function (context, settings) { | ||
| // Wait for DOM update after AJAX or normal load | ||
| setTimeout(function () { | ||
| // Find any element with id starting with 'error-summary-' | ||
| var $error = $('[id^="error-summary-"]', context).first(); | ||
| if ($error.length) { | ||
| $error.attr('tabindex', '-1'); // ensure focusable (in case) | ||
| $error.focus(); | ||
| } | ||
| }, 30); | ||
| } | ||
| }; | ||
| })(jQuery, Drupal); |
There was a problem hiding this comment.
Critical: Code duplication between theme and starter kit.
This file is identical to web/themes/contrib/civictheme/assets/js/status-messages/status-messages.js. Having duplicate code in both the main theme and starter kit creates a maintenance burden where changes must be synchronized across both files.
Recommended approach:
Since this is accessibility-critical functionality, the main theme version should handle it. The starter kit should either:
- Option 1 (Preferred): Remove this file entirely and inherit the behavior from the parent theme.
- Option 2: Only include this file if the starter kit needs to customize or extend the behavior. If so, document why the override is necessary.
If duplication is intentional (e.g., for easier customization), add a comment explaining that this file mirrors the parent theme's version and must be kept in sync:
// This file mirrors the parent theme's status-messages.js
// Keep in sync with: web/themes/contrib/civictheme/assets/js/status-messages/status-messages.js🤖 Prompt for AI Agents
web/themes/contrib/civictheme/civictheme_starter_kit/assets/js/status-messages/status-messages.js
lines 1-16: this file duplicates the identical accessibility-focused
status-messages.js in the parent civictheme, creating maintenance burden; remove
this file from the starter kit so the parent theme's script is used (preferred),
or if the starter kit must provide it, replace its contents with a short comment
stating it intentionally mirrors the parent file and include the parent filepath
for sync tracking, or alternatively keep the file only when adding custom
behavior and document the reason for the override.
| content: message|render, | ||
| type: (type == 'status' or type == 'info') ? 'information' : type, | ||
| vertical_spacing: 'both', | ||
| attributes: id_attr |
There was a problem hiding this comment.
Passing string to attributes may break rendering.
If the civictheme:message template expects an Attribute object or array for the attributes parameter, passing a plain HTML string will likely not render correctly or may be escaped as text rather than merged into the element's attributes.
🤖 Prompt for AI Agents
In web/themes/contrib/civictheme/templates/misc/status-messages.html.twig around
line 16, the template is passing a plain HTML string to the `attributes`
parameter which may break rendering; change the call so `attributes` is an
Attribute object or an associative array (e.g. an attributes object or array
with keys like "id") instead of a raw string, creating or merging the
Attribute/array before passing it to the template and ensuring the id is set as
an attribute (not concatenated HTML), so the rendering engine can properly merge
and render the attributes.
https://www.drupal.org/project/civictheme/issues/3517035
Checklist before requesting a review
Issue #123456 by drupal_org_username: Issue titleChangedsection about WHY something was done if this was not a normal implementationChanged
Screenshots
Summary by CodeRabbit